-
-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enh/liquid motors tank inertia #299
Enh/liquid motors tank inertia #299
Conversation
Currently working on this review but haven't finished yet. Expect to finish it by today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Implementation is looking awesome. I just made a small number of suggestions.
rocketpy/motors/Tank.py
Outdated
def __init__( | ||
self, name, diameter, height, gas, liquid=0, bottomCap="flat", upperCap="flat" | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Add docstring to describe arguments.
rocketpy/motors/Tank.py
Outdated
self.bottomCap = self.capMap.get(self.bottomCap)( | ||
self.diameter / 2, fill_direction="upwards" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add error handling to deal with non-existent cap types?
rocketpy/motors/Tank.py
Outdated
"""Returns the center of mass of the tank's fluids as a function of | ||
time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Specify relative to which point is the center of mass calculated (coordinate system orientation and origin).
rocketpy/motors/Tank.py
Outdated
"""Returns the inertia tensor of the tank's fluids as a function of | ||
time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: make it clear relative to which origin and axes (coordinate system) is this being calculated.
rocketpy/motors/TankGeometry.py
Outdated
a cup of water. Most used for bottom caps. | ||
- "downwards": propellant is filled with the concave site facing down, as | ||
in an inverted cup. Most used for upper caps. | ||
The default is "upwards", and will be used if the user commits a typing error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems dangerous to do this. If the user writes "down", it is use "upwards". I believe implementing error handling would be more appropriate here.
Thank you for the review, great points were made! I will analyse and implement the suggested changes today. |
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
…ocketPy-Team/RocketPy into enh/liquid-motors-tank-inertia
Pull request type
Please check the type of change your PR introduces:
Pull request checklist
Code base additions (for bug fixes / features):
black rocketpy
) has passed locally and any fixes were madepytest --runslow
) have passed locallyWhat is the current behavior?
There is no support for non flat cap inertia evaluation nor the tank gaseous volume.
What is the new behavior?
Expand inertia evaluation for Tanks. The new behavior includes spherical cap inertia calculations and the gaseous portion is taken into account.
Does this introduce a breaking change?